-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(article): Add MD support #1451
Conversation
✅ Deploy Preview for gno-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for teritori-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
12ef521
to
271f701
Compare
2bb197f
to
a8cab39
Compare
a8cab39
to
6408c7a
Compare
packages/screens/FeedNewArticle/components/ArticleContentEditor/utils.ts
Outdated
Show resolved
Hide resolved
In term of UI what do you think about show borders for the editor and the preview ? |
In term of behvior found that enough for a first version and we can iterate on add styles just after. |
it would also be nice (in another PR since it's not the scope) but to use the start of the article to make a preview subtitle and let it optional to write a custom preview, if you agree i will open a new issue |
If you talk about the white borders, it's the same behavior than on other inputs. I try to put this behavior on each input, I love this UI that highlights interactive elements on hover such as inputs or buttons.. In mobile format, I removed the white borders padding, because there is no hover. If to talk about a separation between preview and edition, why not adding one, yes :) Personnaly I prefer pure without separation, it's enough excplicit |
These are draft buttons, I'm looking for better buttons, an other style for these buttons, maybe with icons, something cute. |
…using domVisitors prop
@@ -123,6 +123,8 @@ const gradient = (type: GradientType): LinearGradientProps => { | |||
return getMapPostTextGradient(PostCategory.Normal); | |||
case getMapPostTextGradientType(PostCategory.Article): | |||
return getMapPostTextGradient(PostCategory.Article); | |||
case getMapPostTextGradientType(PostCategory.ArticleMarkdown): | |||
return getMapPostTextGradient(PostCategory.Article); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this on purpose ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have a new PostCategory ArticleMarkdown
(for now), yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completed the description
Please take attention to that: At the final, we'll have only one Article category, type, view, etc. These stuff will be merged with the actual "Article" stuff, and the mention "Markdown" will be removed in the future
packages/utils/feed/markdown.ts
Outdated
fontFamily: "Exo_500Medium", | ||
fontWeight: "500", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make a const for those repeated values ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
a58719b
event.contentOffset.y >= event.contentSize.height - offsetPadding && | ||
isNextPageAvailable.value | ||
) { | ||
fetchNextPage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this code will be triggered every time onScroll event is fired, we should add isFetching flag to block that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FeedPostArticleMarkdownView
is a copy of FeedPostArticleView
.
FeedPostArticleView
will be deleted soon (I hope)
We need to refacto the posts UI code. I don't want this PR being messy, the purpose is just to add a Markdown editor. But it implies to add a new PostCategory, so a new Zod type, so a new FeedPostBlablaView...
And all of these, temporary...
I forgot that could be slow down the review.
I don't know how to avoid adding this kind of copy without adding more complexity that will be removed just after
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completed the description
Please take attention to that: At the final, we'll have only one Article category, type, view, etc. These stuff will be merged with the actual "Article" stuff, and the mention "Markdown" will be removed in the future
}; | ||
|
||
// HTML tags styles used by RenderHtml from react-native-render-html | ||
export const renderHtmlTagStyles: MixedStyleRecord = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can merge and iterate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Needs, todos, etc => https://hackmd.io/@9Oh5kimjS1SP8xUWU_-Ovg/HkcuBoN4yx
We use this library to generate HTML from Markdown syntax: https://github.com/markdown-it/markdown-it
We use this library to render the HTML into JSX: https://github.com/meliorence/react-native-render-html
MarkdownIt
Here, we initialize
MarkdownIt
with some parameters, and use the Articlemessage
to render HTML: https://github.com/TERITORI/teritori-dapp/blob/feat-feed-article-markdown/packages/screens/FeedNewArticle/components/ArticleContentEditor/ArticleContentEditor.tsx#L53We use the same
MarkdownIt
parameters at Article creation and Article consultation (WYSIWYG).RenderHtml
We pass custom styles to
RenderHtml
. Each style item correspond to an html tag. So, there are a lot of customisable styles. We could customise the most used only: https://github.com/TERITORI/teritori-dapp/blob/feat-feed-article-markdown/packages/screens/FeedNewArticle/components/ArticleContentEditor/ArticleContentEditor.tsx#L169Here are the currently customized styles: https://github.com/TERITORI/teritori-dapp/blob/feat-feed-article-markdown/packages/screens/FeedNewArticle/components/ArticleContentEditor/utils.ts#L14-L112
Temporary stuff. Please take attention to that: At the final, we'll have only one Article category, type, view, etc. These stuff will be merged with the actual "Article" stuff, and the mention "Markdown" will be removed in the future
ArticleMarkdown
:teritori-dapp/packages/utils/types/feed.ts
Line 22 in 0aae569
ZodSocialFeedArticleMarkdownMetadata
:teritori-dapp/packages/utils/types/feed.ts
Line 111 in 0aae569
FeedPostArticleMarkdownView
:teritori-dapp/packages/screens/FeedPostView/components/FeedPostArticleMarkdownView.tsx
Line 60 in a58719b
SocialArticleMarkdownCard
:teritori-dapp/packages/components/socialFeed/SocialCard/cards/SocialArticleMarkdownCard.tsx
Line 39 in a58719b